Repository navigation
Conversation
When Bun.serve({http3: true}) succeeds at binding the TCP port but fails
to bind the matching UDP port for QUIC, listen() throws and calls
deinit() on the server. deinit() destroys the uws App, which runs
us_socket_group_deinit() on the HttpContext group. Debug builds abort
there because the TCP listen socket is still linked in
head_listen_sockets; release builds leak the fd (the port stays bound)
and leave the listen socket pointing into freed group storage.
Close the listen socket in deinit() before destroying the App.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough
ChangesTCP socket cleanup on H3 bind failure
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted fix that closes the TCP/H3 listen sockets in deinit() before destroying the uws App, matching the pattern already used in the port-0 retry loop.
Extended reasoning...
Overview
This PR adds two if let Some(...) = take() { close() } blocks to NewServer::deinit() in src/runtime/server/mod.rs, closing the TCP listen socket (and, defensively, the H3 listen socket) before the owning uws App is destroyed. It also adds a regression test in test/js/bun/http/serve-http3.test.ts that forces the TCP-succeeds/UDP-fails path and asserts the TCP port is released.
Correctness
I traced every Self::deinit(this) call site. All the ones in the 2480–2670 range run during setup before listen(), so listener/h3_listener are None. The one inside deinit_if_we_can() (mod.rs:1717) is gated on !has_listener(). The remaining caller at mod.rs:2845 is the post-listen exception path this PR targets — the only place deinit() could be reached with listener == Some. The new code uses .take() so it composes safely with stop_listening(), and the opaque_deref_mut(ls).close() idiom is identical to the existing usage in the port-0 retry loop at mod.rs:2792-2793. Closing the listener before NewApp::destroy is the correct order for us_socket_group_deinit's head_listen_sockets == NULL invariant.
Security risks
None. This is resource-cleanup ordering on an error path; no new inputs, no auth/crypto/parsing changes.
Level of scrutiny
Low–medium. ~11 lines of production code following an established pattern, plus a self-contained subprocess test. The fix is narrowly scoped to a fuzzer-found teardown ordering bug and cannot affect the happy path (listeners are already None there).
Other factors
The test is well-designed: it runs in a subprocess so a debug-build assertion abort wouldn't take down the test runner, asserts on the specific throw message, proves fd release via a TCP rebind, and checks stderr for the assertion string. No CODEOWNERS entry covers this file and there are no outstanding reviewer comments.
When
Bun.serve({http3: true})succeeds at binding the TCP port but fails to bind the matching UDP port for QUIC,listen()throws and callsdeinit()on the server.deinit()destroys the uws App, which runsus_socket_group_deinit()on the HttpContext group.Debug builds abort there because the TCP listen socket is still linked in
head_listen_sockets:Release builds leak the fd (the TCP port stays bound) and leave the listen socket pointing into freed group storage.
Fix: close the listen socket in
deinit()before destroying the App. Every other caller ofdeinit()reaches it withself.listener == None(either listen never ran, ordeinit_if_we_can()gated on!has_listener()), so this only changes the listen-failure path.The test reproduces by reserving a UDP port and asking
Bun.serveto use that exact TCP+UDP port pair; TCP succeeds, UDP fails. It then re-binds TCP on the same port to prove the fd was released.Found by Fuzzilli.